Abstract Ignition operations in a single file #3248

vrutkovs · 2020-03-05T20:14:13Z

This PR moves all functions which create or modify Ignition in a single file and abstract Ignition config in IgnConfig.

The change would enable us to replace Ignition v2 implementation with Ignition v3 using build tags (or any other way on compile-time).

This ensures machineconfigs are created with Ignition 2.2.0 and Openstack shim is created using Ignition v2.4.0.

/cc @LorbusChris @cgwalters @openshift/installer

openshift-ci-robot · 2020-03-05T20:16:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrutkovs · 2020-03-05T20:21:12Z

/cc @smarterclayton @abhinavdahiya

abhinavdahiya · 2020-03-05T20:23:00Z

this is too big a commit to review. can you break it apart into new functions and then moving existing code to it.

also ignConfig isn't a great name.
adding abstractions for dropin, unit, file etc is a little too much for when the installer should actually just move to v3 in one go and the server (here the igntion on the host) is the one that should understand both.

vrutkovs · 2020-03-05T20:31:15Z

this is too big a commit to review. can you break it apart into new functions and then moving existing code to it.

Okay, will do

also ignConfig isn't a great name.

It can be addressed later.

adding abstractions for dropin, unit, file etc is a little too much for when the installer should actually just move to v3 in one go and the server (here the igntion on the host) is the one that should understand both.

That can be done in approximately 4.6 time, when RHCOS switches to Ignition v3. OKD installer would like to do that earlier.

LorbusChris · 2020-03-05T23:36:50Z

/cc @openshift/openshift-team-mco-maintainers

LorbusChris · 2020-03-06T13:08:43Z

pkg/asset/ignition/ignition_v2.go

I opened a PR a month ago to support Spec 2.4 in the provider: hashicorp/terraform-provider-ignition#67

this may not be needed after all. If you don't have to use 2.4 everywhere, I'd say let's not in order to keep changes to a mininum.

LorbusChris · 2020-03-06T13:25:38Z

MCO should probably be updated to natively use Ignition Spec 2.4 before this can go in?
/cc @runcom

LorbusChris · 2020-03-06T23:42:26Z

openshift/machine-config-operator#1543 adds support for Spec 2.4,
but it needs the RawExtension PR first (openshift/machine-config-operator#996)

vrutkovs · 2020-04-06T12:01:16Z

ovirt flaked, workers didn't join on openstack

/retest

vrutkovs · 2020-04-06T17:40:04Z

/hold

Openstack workers don't join the cluster

vrutkovs · 2020-05-16T12:56:24Z

Imagestream import issues
/retest

vrutkovs · 2020-05-19T07:25:24Z

/retest

vrutkovs · 2020-05-21T07:28:36Z

/retest

vrutkovs · 2020-05-26T08:54:32Z

/hold cancel

All issues resolved, PTAL

stbenjam · 2020-05-30T02:08:09Z

/test e2e-metal-ipi

stbenjam · 2020-05-30T17:13:10Z

/test e2e-metal-ipi

vrutkovs · 2020-06-08T12:47:35Z

/retest

LorbusChris · 2020-07-06T06:44:38Z

@vrutkovs do you mind updating this PR once again? Spec 3 support has now merged into MCO, the migration could happen be done any time now.
I propose we collect all ignition-related functions in one file as proposed in this PR (however we won't be needing the build flag), and then flip the switch to v3 in a follow up PR.

vrutkovs · 2020-07-06T09:40:46Z

I propose we collect all ignition-related functions in one file as proposed in this PR (however we won't be needing the build flag), and then flip the switch to v3 in a follow up PR

We still need RHCOS to run Ign3 for tests to pass

This file would contain functions which create or change Ignition files, importing a version of github.com/coreos/ignition

…nal MachineConfigs. Functions from pkg/asset/ignition/node.go are no longer used, so it is removed. `runtime.Extension` is used to be compatible with different Ignition versions

LorbusChris · 2020-07-06T18:18:49Z

/retest

openshift-ci-robot · 2020-07-07T03:01:18Z

@vrutkovs: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`e333587`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-ovirt	`e333587`	link	`/test e2e-ovirt`
ci/prow/e2e-openstack	`e333587`	link	`/test e2e-openstack`
ci/prow/e2e-aws	`e333587`	link	`/test e2e-aws`
ci/prow/verify-codegen	`e333587`	link	`/test verify-codegen`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vrutkovs · 2020-07-10T08:13:46Z

Superceded by #3871

openshift-ci-robot requested review from a team, LorbusChris and cgwalters March 5, 2020 20:14

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2020

openshift-ci-robot requested review from abhinavdahiya and smarterclayton March 5, 2020 20:21

vrutkovs force-pushed the master-ign-abstraction branch from 16f9110 to c785f43 Compare March 5, 2020 20:45

LorbusChris reviewed Mar 6, 2020

View reviewed changes

openshift-ci-robot requested a review from runcom March 6, 2020 13:25

LorbusChris mentioned this pull request Mar 7, 2020

Update to Ignition 0.35.0 / Spec 2.4 openshift/machine-config-operator#1543

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2020

vrutkovs force-pushed the master-ign-abstraction branch from c785f43 to 08305f9 Compare March 10, 2020 09:08

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2020

vrutkovs force-pushed the master-ign-abstraction branch 5 times, most recently from 0d3c284 to 3de649f Compare April 6, 2020 08:19

vrutkovs changed the title ~~WIP Abstract Ignition operations in a single file~~ Abstract Ignition operations in a single file Apr 6, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020

vrutkovs mentioned this pull request May 19, 2020

Add "Build OKD-on-Fedora-CoreOS in Prow" proposal openshift/enhancements#78

Merged

vrutkovs force-pushed the master-ign-abstraction branch from 1383b6f to 60fa219 Compare May 20, 2020 18:16

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2020

vrutkovs force-pushed the master-ign-abstraction branch from 60fa219 to f4fb1b6 Compare May 29, 2020 22:17

vrutkovs force-pushed the master-ign-abstraction branch from f4fb1b6 to c5ede55 Compare June 8, 2020 07:52

vrutkovs force-pushed the master-ign-abstraction branch from c5ede55 to f9e7abd Compare June 9, 2020 11:36

vrutkovs force-pushed the master-ign-abstraction branch from f9e7abd to 4fc5d48 Compare July 1, 2020 09:12

Vadim Rutkovsky added 9 commits July 6, 2020 11:50

pkg/asset/ignition: create functions which create or modify Ignition

15afe63

This file would contain functions which create or change Ignition files, importing a version of github.com/coreos/ignition

pkg/asset/cluster: use InjectInstallInfo from pkg/asset/ignition

6817ba9

pkg/tfvars/openstack: use GenerateIgnitionShim from ignition pkg

1df56ea

pkg/asset/ignition/machine: use PointerIgnitionConfig from ignition pkg

7816887

pkg/assets/ignition/bootstrap: use functions from ignition pkg

7c81361

pkg/asset/machines: use functions in ignition pkg to generate additio…

28ebe9c

…nal MachineConfigs. Functions from pkg/asset/ignition/node.go are no longer used, so it is removed. `runtime.Extension` is used to be compatible with different Ignition versions

ignition_v2: use ignition 2.2 for most files and 2.4 for Openstack shim

f6cede3

Bump to latest MCO

e89e321

go mod vendor

e333587

vrutkovs force-pushed the master-ign-abstraction branch from 4fc5d48 to e333587 Compare July 6, 2020 09:50

vrutkovs closed this Jul 10, 2020

Abstract Ignition operations in a single file #3248

Abstract Ignition operations in a single file #3248

Uh oh!

Conversation

vrutkovs commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 5, 2020

Uh oh!

vrutkovs commented Mar 5, 2020

Uh oh!

abhinavdahiya commented Mar 5, 2020

Uh oh!

vrutkovs commented Mar 5, 2020

Uh oh!

LorbusChris commented Mar 5, 2020

Uh oh!

LorbusChris Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

LorbusChris Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

LorbusChris commented Mar 6, 2020

Uh oh!

LorbusChris commented Mar 6, 2020

Uh oh!

vrutkovs commented Apr 6, 2020

Uh oh!

vrutkovs commented Apr 6, 2020

Uh oh!

vrutkovs commented May 16, 2020

Uh oh!

vrutkovs commented May 19, 2020

Uh oh!

vrutkovs commented May 21, 2020

Uh oh!

vrutkovs commented May 26, 2020

Uh oh!

stbenjam commented May 30, 2020

Uh oh!

stbenjam commented May 30, 2020

Uh oh!

vrutkovs commented Jun 8, 2020

Uh oh!

LorbusChris commented Jul 6, 2020

Uh oh!

vrutkovs commented Jul 6, 2020

Uh oh!

LorbusChris commented Jul 6, 2020

Uh oh!

openshift-ci-robot commented Jul 7, 2020

Uh oh!

vrutkovs commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vrutkovs commented Mar 5, 2020 •

edited

Loading